Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[B5] Support for migrating report views to Bootstrap 5 #35648

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

biyeun
Copy link
Member

@biyeun biyeun commented Jan 21, 2025

Technical Summary

Recommended: Review Commit-by-Commit

This PR does a few things, but it does not migrate ANY existing reports yet (example migration is here). I carefully made sure the changes in this PR would NOT threaten production workflows, so that this PR is safe to merge without QA. I have put the branch, including two report migrations (ApplicationStatusReport and CaseListReport) on staging.

This PR:

  1. Sets up the infrastructure behind Report Views to support migrating that report to bootstrap5. This means a way to enable a bootstrap 5 mode for the report, in this case setting use_bootstrap5 = True for that report view. This makes sure that bootstrap5 versions of base report and filter templates are correctly loaded.

  2. Upgrades datatables_config to be compatible with Datatables 1.10+ style parameters. We need to use a new version of Datatables in order to support bootstrap 5 styling. The new version of datatables introduces SIGNIFICANT changes into how the request QueryString object looks when a datatables-initiated request is made. These changes means I had to get creative by creating a request parameter "interpreter" in order to get the desired value from filter parameters inserted into a datatables pagination request.

  3. Updates HTML formatting and any bootstrap 5 related updates to core/base report base templates and filters. This PR does NOT make required updates to ALL filters and report templates. The idea is that these templates will be updated as each report is migrated. See this diff for an example of what migrating two reports (ApplicationStatusReport and CaseListReport) looks like:
    bmb/b5-reports...bmb/b5-reports-initial-migration

  4. Updates core javascript so that the most basic filters/reports can load without error. I tested the JS updates with migrating CaseListReport and ApplicationStatusReport (has an email report button). I very INTENTIONALLY left off the work needed to migrate datespan filters, as I was worried that work would not allow me to finish this PR in the time I had set aside for it as part of dependacat duties. My concern is that the datespan filter javascript needs to move toward using Tempus Dominus, and I think there might be additional tweaks required. I wanted to narrow the scope of this PR to fixing the datatables 1.10+ and report view setup to handle loading ANY report.

  5. I introduced debugging tools (activated by setting debug_bootstrap5 = True on the report) to make sure it's easier to figure out what filters and templates need migration—and to ensure no feature flagged filters are missed! Alongside this work is a new migration helper tool ./manage.py complete_bootstrap5_report <ReportClassName> to keep track of what reports, filters, and filter templates have already been migrated. It also ensures that child reports are non un-intentionally migrated (e.g. CaseListExplorer, which inherits from CaseListReport is not migrated).

  6. Lastly, updates to the Bootstrap 5 Migration guide for migrating report views with a 4-step routine to ensure a report view is fully migrated to Bootstrap 5.

Read the Bootstrap 5 Migration Guide additions here

Safety Assurance

Safety story

The changes here were carefully applied so as to not affect production workflows. I've put the branch on staging to ensure core functionality is not affected.

Automated test coverage

Yes, where appropriate I've added tests.

QA Plan

I believe the changes here are isolated enough that QA is not needed, however I will be triggering QA for the first two migrated reports here, which will come as a later PR. I hope to not keep this PR unmerged for too long to avoid merge conflicts with folks working in overlapping areas.

Rollback instructions

depending on how long we wait to roll back, the bootstrap 5 diffs might make it cumbersome to revert as a whole. However each individual commit is revertable.

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@biyeun biyeun added product/invisible Change has no end-user visible impact dependencies/javascript Change in javascript dependency. labels Jan 21, 2025
@biyeun biyeun requested review from millerdev, orangejenny and a team January 21, 2025 15:58
@dimagimon dimagimon added the Risk: Medium Change affects files that have been flagged as medium risk. label Jan 21, 2025
@biyeun biyeun changed the title [B5] Support for migrating report views to [B5] Support for migrating report views to Bootstrap 5 Jan 21, 2025
@biyeun
Copy link
Member Author

biyeun commented Jan 21, 2025

I intentionally ignored un-related code that had lint flags because it resulted in unnecessary merge conflicts with current work on staging, as I had to touch quite a few different files with the request_params changes

return instance.__class__.__name__


def reports_bootstrap5_template_debugger(report_instance):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example output of what it looks like in the console:

Screenshot 2025-01-21 at 3 25 59 PM
Screenshot 2025-01-21 at 3 24 51 PM

from corehq.motech.repeaters.views import DomainForwardingRepeatRecords


ALL_REPORTS = [
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too concerned about enforcing that this list is kept up-to-date as new reports are added. I think it will be clear to anyone who goes to a report missing in this list to add it here if they migrate the report and try to run complete_bootstrap5_report TheirReportView.

@@ -31,6 +31,7 @@ <h5 class="my-2 ms-3">On this page</h5>
<li><a href="#un-split-view-files">Un-Split View Files</a></li>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if self.asynchronous:
return self.base_template_async or "reports/async/bootstrap3/default.html"
return self.base_template_async or default_path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this use get_bootstrap5_path?

mark_report_as_complete(report_class.__name__)
self.suggest_commit_message(f"Completed report: {report_class.__name__}.")

def is_filter_migrated(self, field, filter_class, migrated_filter_templates):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more descriptive name, like flag_unmigrated_filter or is_filter_migrated_prompt?

@biyeun
Copy link
Member Author

biyeun commented Jan 22, 2025

^removed the build bootstrap5 diffs commit as that's going to cause a few merge conflicts with the smaller PRs i'm branching off from this!

@orangejenny
Copy link
Contributor

#35651 should resolve the javascript lint errors (although it will likely also introduce new ones)

@dimagimon dimagimon removed the Risk: Medium Change affects files that have been flagged as medium risk. label Jan 22, 2025
@biyeun biyeun force-pushed the bmb/b5-reports branch 2 times, most recently from 665fc6a to e7f2fa8 Compare January 23, 2025 12:36
@biyeun
Copy link
Member Author

biyeun commented Jan 23, 2025

NOTE: I'm actively rebasing the commits here to reduce the size of this PR and move commits out of it into smaller PRs. we will resume group review on monday :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies/javascript Change in javascript dependency. product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants